Metrics: export evcc state on /metrics#31602
Conversation
The --metrics endpoint so far only served Go runtime metrics. This adds a third publisher (next to mqtt.go and influxdb.go) that mirrors the internal state into Prometheus gauges, so /metrics also serves evcc data (grid/pv/ battery power, SoC, loadpoint values, ...). Reuses the existing --metrics flag, no new config.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
record, the struct-field iterationfor i := range typ.NumField()is incorrect and will never iterate fields; change this to a standard indexed loop likefor i := 0; i < typ.NumField(); i++ { ... }so struct fields are actually exported. - In
Collect, label names and values are built by ranging over thelabelsmap, which has nondeterministic order; sort keys before constructinglabelNames/labelValuesto avoid intermittent descriptor/consistency issues withclient_golangwhen multiple samples share the same metric name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `record`, the struct-field iteration `for i := range typ.NumField()` is incorrect and will never iterate fields; change this to a standard indexed loop like `for i := 0; i < typ.NumField(); i++ { ... }` so struct fields are actually exported.
- In `Collect`, label names and values are built by ranging over the `labels` map, which has nondeterministic order; sort keys before constructing `labelNames`/`labelValues` to avoid intermittent descriptor/consistency issues with `client_golang` when multiple samples share the same metric name.
## Individual Comments
### Comment 1
<location path="server/prometheus.go" line_range="188-189" />
<code_context>
+ }
+ }
+
+ case reflect.Slice, reflect.Array:
+ for i := range rv.Len() {
+ l := maps.Clone(labels)
+ l["id"] = strconv.Itoa(i + 1)
</code_context>
<issue_to_address>
**issue (bug_risk):** Slice/array iteration also uses `range` over an `int`, which will not compile; switch to a standard index loop.
You can implement it like this:
```go
case reflect.Slice, reflect.Array:
for i := 0; i < rv.Len(); i++ {
l := maps.Clone(labels)
l["id"] = strconv.Itoa(i + 1)
p.record(key, rv.Index(i).Interface(), l)
}
```
`rv.Len()` returns an `int`, so it must be used as the loop bound, not as the operand to `range`.
</issue_to_address>
### Comment 2
<location path="server/prometheus.go" line_range="30" />
<code_context>
+var invalidPrometheusChars = regexp.MustCompile(`[^a-zA-Z0-9]+`)
+
+// promSample is the last known value for a single metric+label combination
+type promSample struct {
+ value float64
+ labels prometheus.Labels
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring how labels are stored and used so that ordered label name/value slices are created once and reused, rather than rebuilding and reordering label maps on every scrape.
You can reduce complexity and a potential correctness risk around labels by avoiding rebuilding label names/values and re‑deriving order on every scrape.
Right now you:
- Build a `labelSignature` by sorting a `map[string]string` (stable order),
- Store `promSample` with a `map[string]string`,
- In `Collect` you reconstruct `labelNames`/`labelValues` from the map with *non‑deterministic* ordering.
This makes label ordering implicit and fragile and forces extra work on every scrape.
You can simplify this by:
1. Storing ordered label names/values once when setting a sample.
2. Deriving the signature from those slices.
3. Using those slices directly in `Collect`.
### Suggested refactor
Change `promSample` to hold ordered slices:
```go
type promSample struct {
value float64
labelNames []string
labelValues []string
}
```
Add a helper to normalize labels once:
```go
func normalizeLabels(labels prometheus.Labels) ([]string, []string) {
if len(labels) == 0 {
return nil, nil
}
names := make([]string, 0, len(labels))
for k := range labels {
names = append(names, k)
}
sort.Strings(names)
values := make([]string, len(names))
for i, k := range names {
values[i] = labels[k]
}
return names, values
}
func labelSignature(names, values []string) string {
var b strings.Builder
for i, name := range names {
b.WriteString(name)
b.WriteByte('=')
b.WriteString(values[i])
b.WriteByte(';')
}
return b.String()
}
```
Update `set` to compute everything once:
```go
func (p *Prometheus) set(name string, value float64, labels prometheus.Labels) {
p.mu.Lock()
defer p.mu.Unlock()
if p.samples[name] == nil {
p.samples[name] = make(map[string]promSample)
}
names, vals := normalizeLabels(labels)
sig := labelSignature(names, vals)
p.samples[name][sig] = promSample{
value: value,
labelNames: names,
labelValues: vals,
}
}
```
`Collect` can then be much simpler and avoids recomputing/scrambling label order:
```go
func (p *Prometheus) Collect(ch chan<- prometheus.Metric) {
p.mu.Lock()
defer p.mu.Unlock()
for name, samples := range p.samples {
for _, s := range samples {
desc := prometheus.NewDesc(name, "evcc: "+name, s.labelNames, nil)
m, err := prometheus.NewConstMetric(desc, prometheus.GaugeValue, s.value, s.labelValues...)
if err != nil {
p.log.ERROR.Printf("collect %s: %v", name, err)
continue
}
ch <- m
}
}
}
```
This keeps all existing functionality but:
- Removes the need for a label map in `promSample`,
- Ensures stable label ordering (and thus consistent series identity),
- Eliminates repeated map iteration / slice construction in `Collect`,
- Tightens the path from input labels → signature → output metric.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| case reflect.Slice, reflect.Array: | ||
| for i := range rv.Len() { |
There was a problem hiding this comment.
issue (bug_risk): Slice/array iteration also uses range over an int, which will not compile; switch to a standard index loop.
You can implement it like this:
case reflect.Slice, reflect.Array:
for i := 0; i < rv.Len(); i++ {
l := maps.Clone(labels)
l["id"] = strconv.Itoa(i + 1)
p.record(key, rv.Index(i).Interface(), l)
}rv.Len() returns an int, so it must be used as the loop bound, not as the operand to range.
| var invalidPrometheusChars = regexp.MustCompile(`[^a-zA-Z0-9]+`) | ||
|
|
||
| // promSample is the last known value for a single metric+label combination | ||
| type promSample struct { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring how labels are stored and used so that ordered label name/value slices are created once and reused, rather than rebuilding and reordering label maps on every scrape.
You can reduce complexity and a potential correctness risk around labels by avoiding rebuilding label names/values and re‑deriving order on every scrape.
Right now you:
- Build a
labelSignatureby sorting amap[string]string(stable order), - Store
promSamplewith amap[string]string, - In
Collectyou reconstructlabelNames/labelValuesfrom the map with non‑deterministic ordering.
This makes label ordering implicit and fragile and forces extra work on every scrape.
You can simplify this by:
- Storing ordered label names/values once when setting a sample.
- Deriving the signature from those slices.
- Using those slices directly in
Collect.
Suggested refactor
Change promSample to hold ordered slices:
type promSample struct {
value float64
labelNames []string
labelValues []string
}Add a helper to normalize labels once:
func normalizeLabels(labels prometheus.Labels) ([]string, []string) {
if len(labels) == 0 {
return nil, nil
}
names := make([]string, 0, len(labels))
for k := range labels {
names = append(names, k)
}
sort.Strings(names)
values := make([]string, len(names))
for i, k := range names {
values[i] = labels[k]
}
return names, values
}
func labelSignature(names, values []string) string {
var b strings.Builder
for i, name := range names {
b.WriteString(name)
b.WriteByte('=')
b.WriteString(values[i])
b.WriteByte(';')
}
return b.String()
}Update set to compute everything once:
func (p *Prometheus) set(name string, value float64, labels prometheus.Labels) {
p.mu.Lock()
defer p.mu.Unlock()
if p.samples[name] == nil {
p.samples[name] = make(map[string]promSample)
}
names, vals := normalizeLabels(labels)
sig := labelSignature(names, vals)
p.samples[name][sig] = promSample{
value: value,
labelNames: names,
labelValues: vals,
}
}Collect can then be much simpler and avoids recomputing/scrambling label order:
func (p *Prometheus) Collect(ch chan<- prometheus.Metric) {
p.mu.Lock()
defer p.mu.Unlock()
for name, samples := range p.samples {
for _, s := range samples {
desc := prometheus.NewDesc(name, "evcc: "+name, s.labelNames, nil)
m, err := prometheus.NewConstMetric(desc, prometheus.GaugeValue, s.value, s.labelValues...)
if err != nil {
p.log.ERROR.Printf("collect %s: %v", name, err)
continue
}
ch <- m
}
}
}This keeps all existing functionality but:
- Removes the need for a label map in
promSample, - Ensures stable label ordering (and thus consistent series identity),
- Eliminates repeated map iteration / slice construction in
Collect, - Tightens the path from input labels → signature → output metric.
|
On the first one — On the label ordering: names and values are appended in the same loop, so they never get out of sync, and client_golang sorts the label pairs itself before writing them — so the output stays stable (there's a test for the mixed-label case too). Happy to move the slice-building into |
|
I still don‘t think this is a good idea:
from my personal pov wontfix unless and before we have a proper metrics system. |
The "--metrics" endpoint so far only exposed Go runtime metrics. This adds
evcc metrics (grid/pv/battery power, SoC, loadpoint values, ...) to
the same endpoint.
Addressing the earlier concerns in #467:
Example: ( full example: metrics.txt )
evcc_grid_power -705
evcc_pv_power 1341
evcc_battery_soc 100
evcc_loadpoint_charge_power{loadpoint="Garage"} 0
evcc_grid_currents{phase="1"} 2.3
All values are gauges, including monotonic ones like "charged_energy" — a
Tested against a real setup (AlphaESS inverter + battery, Juice ChargeMe
charger), a second run on a different setup would be welcome.